Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add Reviewdog as checkstyle #6996

Merged
merged 9 commits into from
Oct 24, 2020
Merged

Add Reviewdog as checkstyle #6996

merged 9 commits into from
Oct 24, 2020

Conversation

Siedlerchr
Copy link
Member

@Siedlerchr Siedlerchr commented Oct 9, 2020 •
edited
Loading

Motivation: Many new contributors seem to ignore or don't see the failing github checks

  • Change in CHANGELOG.md described (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

@Siedlerchr Siedlerchr closed this Oct 9, 2020
@Siedlerchr Siedlerchr mentioned this pull request Oct 9, 2020
Closed
@Siedlerchr Siedlerchr reopened this Oct 16, 2020
@Siedlerchr
Merge remote-tracking branch 'upstream/master' into checkstyleConfig
489cac4 
* upstream/master:
  Update journalList.mv
  Update to javafx15 (#7018)
  Squashed 'src/main/resources/csl-styles/' changes from 6fab78b..5297abd
  try to fix DEP issue with official jdk (#7008)
  Jstor Fetcher (#6992)
  Group: "Searching for keywords" searches for a single keyword ==> use singular (#6995)
  Merge parsing of bracketed patterns (#6989)
  6848 fixed the issue of clicking collapse all expanding tree (#6993)
  Enable auto sync per default for Open/Libre Office (#6985)
  Bump unirest-java from 3.11.00 to 3.11.01 (#7001)
  Bump byte-buddy-parent from 1.10.16 to 1.10.17 (#7004)
  Bump lucene-queryparser from 8.6.2 to 8.6.3 (#7002)
  Bump postgresql from 42.2.16 to 42.2.17 (#7005)
  Bump pascalgn/automerge-action from v0.11.0 to v0.12.0 (#7006)
  Bump flowless from 0.6.1 to 0.6.2 (#7003)
@Siedlerchr
test witout gradle action
dee6594
@Siedlerchr
test checkstyle
4c06293
@Siedlerchr
use copy for reviewdog
f019ed1 
change to PR reporter
@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Oct 16, 2020
tobiasdiez
tobiasdiez reviewed Oct 17, 2020
View reviewed changes
Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a big fan that reviewdog add these warnings as comments. Github has check annotation exactly for this purpose, so I would prefer if that could be used, e.g. https://github.com/staabm/annotate-pull-request-from-checkstyle (not sure if this also works for java-checkstyle).

config/checkstyle/checkstyle_reviewdog.xml
@@ -0,0 +1,141 @@
<?xml version="1.0"?>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need to duplicate the checkstyle config? Makes it hard in the future to keep them synced

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checks are also possible aka the default, but only visible if you look at the files changed tab.
I personally think this comment variant is useful

Unfortunately we currently need this duplication because otherwise the suppression config file cannot be found due to the parameter expansion or the gradle check cannot be run.
There's also no way to inline the suppression

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It just adds so much noise, if you commit half-finished PRs that still need cleanup etc. You then get github notifications because a bot commented. Why not educate people to look at all checks?

Currently, the checkstyle issues do not show up in the changed files tab.

So with the current config, reviewdog will report about things that we actually decided to suppress?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed back to github checks feature.

Again, for the copy. Reviewdog needs a copy of the checkstyle.xml because it contains the relative path to the supression.xml file. Otherwise it cannot parse the ${configloc} variable.
Gradle and the IDEs need that variable to automatically set the correct path

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Can you then please open an issue at reviewdog so that this gets fixed in the future. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's more a checkstlye issue (that won't be fixed). However, I opened an issue at the action repo.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is nikitasavinov/checkstyle-action#16. It should have been solved with nikitasavinov/checkstyle-action#19. Should we try it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try it...

@tobiasdiez tobiasdiez added the status: changes required Pull requests that are not yet complete label Oct 18, 2020
@tobiasdiez tobiasdiez removed the status: changes required Pull requests that are not yet complete label Oct 18, 2020
@tobiasdiez
Copy link
Member

Can reviewdog also parse Junit test reports? If not it would be good to integrate something as https://github.com/marketplace/actions/junit-report-to-annotations

@Siedlerchr
Copy link
Member Author

@tobiasdiez https://github.com/reviewdog/reviewdog#input-format
Not yet as far as I see:
reviewdog/reviewdog#747

@tobiasdiez
Copy link
Member

Ok, can you then please play around with the junit annotation action I posted above. I think this is actually more important than checkstyle.

@Siedlerchr
Copy link
Member Author

I fear the action you posted will not work on forks (due to Github acess token etc)

@Siedlerchr Siedlerchr merged commit 2555b2d into master Oct 24, 2020
@Siedlerchr Siedlerchr deleted the checkstyleConfig branch October 24, 2020 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@koppor koppor koppor left review comments

@tobiasdiez tobiasdiez tobiasdiez approved these changes

Assignees
No one assigned
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Siedlerchr @tobiasdiez @koppor